Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

File list sorting by clicking on column headers #8041

Merged
merged 7 commits into from May 12, 2014
Merged

Conversation

PVince81
Copy link
Contributor

@PVince81 PVince81 commented Apr 3, 2014

File list sorting by clicking on column headers.

This PR will contain file list sorting.

  • Sortable file list
  • Sortable trashbin
  • Sortable public page
  • Upload file while sorted must appear at the correct position
  • Sort indicator on headers (arrows?)
  • Clean up server-side comparators ?

@PVince81
Copy link
Contributor Author

PVince81 commented Apr 3, 2014

  • Client side unit test for sort/reload
  • Server side unit test for getFiles() with different sort values

@ghost
Copy link

ghost commented Apr 3, 2014

💣 Test Failed. 💣
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4052/

@ghost
Copy link

ghost commented Apr 4, 2014

💣 Test Failed. 💣
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4060/

@ghost
Copy link

ghost commented Apr 9, 2014

💣 Test Failed. 💣
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4176/

@PVince81 PVince81 changed the title [WIP] File list sorting File list sorting by clicking on column headers Apr 11, 2014
@PVince81
Copy link
Contributor Author

Fixes #164 #6978

Also TODO:

  • make sorting by date the default in the trashbin

@PVince81
Copy link
Contributor Author

@jancborchardt please have a look, you might want to tweak the styles a bit.
I've added a mouse hover on the column headers and also the triangle icon.

@PVince81
Copy link
Contributor Author

@DeepDiver1975 @icewind1991 I'm finished with this branch, but it is based off infinite scrolling, which means it will need rebase after infinite scroll is merged.

@ghost
Copy link

ghost commented Apr 11, 2014

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4226/

@ghost
Copy link

ghost commented Apr 11, 2014

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4228/

@PVince81
Copy link
Contributor Author

CC @MTRichards

@MTRichards
Copy link
Contributor

Awesome! :)

@icewind1991
Copy link
Contributor

Works great, but why is sorting done server side instead of client side?

@vgezer
Copy link
Contributor

vgezer commented Apr 16, 2014

Looks really nice :), but it seems some special characters in languages are not sorted properly. I think it was referred to: #7254

@jancborchardt jancborchardt added this to the ownCloud 7 milestone Apr 17, 2014
@PVince81
Copy link
Contributor Author

@icewind1991 client side might be slower especially with many files. Also in the future we might want to have the sorting be done by the database engine, and also the paging (only loading a few results, not the whole list) on the server side as well.

@wakeup yes, you're right.

@PVince81
Copy link
Contributor Author

Rebased. This is ready for review + testing.

@ghost
Copy link

ghost commented Apr 28, 2014

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4482/

@PVince81
Copy link
Contributor Author

@DeepDiver1975 feel free to hijack this PR... oops too late you already did it. 😉
Just wanted to check whether it was intentional and you didn't submit to the wrong one ?

@PVince81
Copy link
Contributor Author

@icewind1991 see my comment above about server side sorting. Are you ok with that approach ? Thanks.

@DeepDiver1975
Copy link
Member

Just wanted to check whether it was intentional and you didn't submit to the wrong one ?

All intentional here 😉

@icewind1991
Copy link
Contributor

If paging is done server side then sorting should obviously also be done server side but as long as the server sends the full filelist anyway, I would think client side sorting would always be faster since you don't need have an extra ajax request or query the database

@PVince81
Copy link
Contributor Author

I think the extra ajax request is acceptable when clicking on headers.
Some browsers like IE8 might be quite slow for sorting longer file lists (1000+) on the client-side initially before displaying them.

But one advantage of client-side sorting only would be to have consistent sort, always using an algo on the browser (locale/natural sort)

I would hope that the sorting would be done on the SQL level, but due to the natural sort algos we want to use (and putting the directories first) it is not yet possible.

@ghost
Copy link

ghost commented Apr 29, 2014

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4517/

@scrutinizer-notifier
Copy link

The inspection completed: 8 new issues, 16 updated code elements

@PVince81
Copy link
Contributor Author

@icewind1991 besides IE8 there are also mobile browsers which might be slow as well to sort longer list of files.

@PVince81
Copy link
Contributor Author

PVince81 commented May 2, 2014

@icewind1991 I don't want to throw away the server side sort code just yet for the reasons mentionned above.
It is possible to make it work on the client side with 2-3 extra JS lines later if we ever want it later.
I might add this as part of the "sharing overview" task (on a separate branch) because it uses the OCS Share API that doesn't return sorted results.

@PVince81
Copy link
Contributor Author

PVince81 commented May 2, 2014

To clarify: the "sharing overview page" uses a subclass of FileList and displays a list of shared files.

@Nowaker
Copy link

Nowaker commented May 5, 2014

Any chance it gets merged into version 6?

@karlitschek
Copy link
Contributor

No. This is a complex change that will be part of owncloud 7. We only backport Bugfixes. But you can use the master daily builds as a preview.

@karlitschek karlitschek closed this May 5, 2014
@DeepDiver1975 DeepDiver1975 reopened this May 5, 2014
@karlitschek
Copy link
Contributor

Ups. Wrong button. Thanks @DeepDiver1975 :-)

@ghost
Copy link

ghost commented May 5, 2014

💣 Test Failed. 💣
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4556/

@DeepDiver1975
Copy link
Member

@karlitschek np 😉

@DeepDiver1975
Copy link
Member

@owncloud-bot retest this please

@ghost
Copy link

ghost commented May 5, 2014

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4557/

@PVince81
Copy link
Contributor Author

PVince81 commented May 7, 2014

As discussed with @karlitschek, server-side sort is the way to go for the files app.

@PVince81
Copy link
Contributor Author

@icewind1991 can we merge this now ?

@icewind1991
Copy link
Contributor

Yes 👍

@PVince81
Copy link
Contributor Author

@jancborchardt I didn't receive any feedback about the design so I will merge this first.
The design / look can be tweaked in separate PRs.

PVince81 pushed a commit that referenced this pull request May 12, 2014
File list sorting by clicking on column headers
@PVince81 PVince81 merged commit 9a9665f into master May 12, 2014
@PVince81 PVince81 deleted the files-sortcolumns branch May 12, 2014 10:50
@jbtbnl jbtbnl mentioned this pull request May 12, 2014
4 tasks
@jancborchardt
Copy link
Member

@PVince81 better always mention @owncloud/designers instead of just me alone as otherwise I’ll drown in issue notifications. ;)

Anyway, see @jbtbnl’s review at #8551

@lock lock bot locked as resolved and limited conversation to collaborators Aug 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants